BF+TST: test /fix for signed int data type #302
Conversation
We were incorrectly loading signed ints as unsigned; fix and test.
|
Any reviews here? I think this is a bug and I will merge soon unless I hear otherwise. |
| (4, 'ECAT7_VAXR4', np.float32), | ||
| (5, 'ECAT7_IEEER4', np.float32), | ||
| (6, 'ECAT7_SUNI2', np.uint16), | ||
| (6, 'ECAT7_SUNI2', np.int16), |
There was a problem hiding this comment.
These do indeed look like they were wrong. Any idea why the original author would have head float32 for a datatype that has I4 at the end and uint16 when everything else is int*?
There was a problem hiding this comment.
No idea I'm afraid. I don't think I've got any real data to test with, for ECAT_VAXI4 either, so it's only a guess.
There was a problem hiding this comment.
There was a problem hiding this comment.
Don't know if it's right, but it's at least another reference
There was a problem hiding this comment.
...and it seems to match what you've done here in any case
There was a problem hiding this comment.
Aha - yes - thanks for tracking that down.
There was a problem hiding this comment.
Hmm - this:
http://www.turkupetcentre.net/software/libdoc/libtpcimgio/ecat7r_8c_source.html#l00720
suggests to me that the BYTE type is signed - do you agree?
There was a problem hiding this comment.
The pointer starts out as a char * and isn't re-cast as anything else, which makes me think it is unsigned. What made you think it was signed?
There was a problem hiding this comment.
Just some trivial testing with gcc:
#include <limits.h>
int main() {
printf("Char min is %d\n", CHAR_MIN);
}
But I see in fact it is undefined ...
http://stackoverflow.com/questions/2054939/is-char-signed-or-unsigned-by-default
There was a problem hiding this comment.
The only ECAT files I can find on our system have ECAT7_SUNI2 type, so very hard to know what the intention was.
|
LGTM, +1 for merge. My comment above is just a question for my own edification. |
Eric Larson tracked down the relevant documentation for the datatypes. We discovered that ECAT7_BYTE might be signed int, but we have no data to test with, so leaving as is for now.
|
Thanks - I added a few lines pointing to the docs you found, merging. |
BF+TST: test /fix for signed int data type We were incorrectly loading signed ints as unsigned; fix and test. See gh-297 for motivation.
BF+TST: test /fix for signed int data type We were incorrectly loading signed ints as unsigned; fix and test. See nipygh-297 for motivation.
We were incorrectly loading signed ints as unsigned; fix and test.
See : #297 for motivation.